api: LEDS api introduction by adisuissa · Pull Request #17419 · envoyproxy/envoy

adisuissa · 2021-07-20T11:25:32Z

Commit Message: Adding the LEDS api
Additional Description:
This is the first part of the work on #10373.
Here we introduce the API changes in the load assignments configurations.
LEDS is only supported in delta-xDS (incremental).

Risk Level: Low - api changes only.
Testing: None - api changes only.
Docs Changes: None at the moment.
Release Notes: None at the moment.
Platform Specific Features: N/A.

Signed-off-by: Adi Suissa-Peleg adip@google.com

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2021-07-20T11:25:40Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17419 was opened by adisuissa.

see: more, trace.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

markdroth · 2021-07-21T16:51:48Z

api/envoy/config/endpoint/v3/endpoint_components.proto

  // The group of endpoints belonging to the locality specified.
-  repeated LbEndpoint lb_endpoints = 2;
+  repeated LbEndpoint lb_endpoints = 2
+      [(udpa.annotations.field_migrate).oneof_promotion = "lb_config"];


I don't think it's legal to have a repeated field in a oneof, so this oneof_promotion won't actually work. :(

Yes you are correct, thanks!
I think the right way would be to support both lb_endpoints and the leds_cluster_locality_config (similar to what's being done for VHDS). I've looked at my implementation and it won't require much change.

Alternatively, consider deprecating the existing lb_endpoints field and replacing it with a message that contains the repeated field, and then putting that message in the oneof. In other words, add this message:

message LbEndpoints { repeated LbEndpoint lb_endpoints = 1; }

And then change the fields here to:

// Deprecated -- use inline_endpoints instead. repeated LbEndpoint lb_endpoints = 2 [deprecated = true]; oneof endpoints { LbEndpoints inline_endpoints = 7; LedsClusterLocalityConfig leds_cluster_locality_config = 8; }

markdroth · 2021-07-21T16:52:45Z

/wait

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

markdroth

/wait

markdroth · 2021-07-21T18:47:01Z

api/envoy/config/endpoint/v3/endpoint_components.proto

  // The group of endpoints belonging to the locality specified.
-  repeated LbEndpoint lb_endpoints = 2;
+  repeated LbEndpoint lb_endpoints = 2
+      [(udpa.annotations.field_migrate).oneof_promotion = "lb_config"];


Alternatively, consider deprecating the existing lb_endpoints field and replacing it with a message that contains the repeated field, and then putting that message in the oneof. In other words, add this message:

message LbEndpoints { repeated LbEndpoint lb_endpoints = 1; }

And then change the fields here to:

// Deprecated -- use inline_endpoints instead. repeated LbEndpoint lb_endpoints = 2 [deprecated = true]; oneof endpoints { LbEndpoints inline_endpoints = 7; LedsClusterLocalityConfig leds_cluster_locality_config = 8; }

adisuissa · 2021-07-21T19:03:33Z

Thanks for the suggestion.
I've considered this, and I'm not sure if the deprecation churn is worth the price (although I might be wrong, and would prefer to hear this now :) ).
On the one hand, a single source of truth where all the endpoints are located, either as a list in an EDS response or a list coming from LEDS, makes sense to me from operational point-of-view. On the other hand, to reduce deprecation churn and to align with VHDS allowing both fields concurrently is probably the right way to go.
Feel free to tell me if you think differently.

BTW: I've only looked at how Envoy's implementation handles this (which supporting both should not be hard), but not sure about other clients.

markdroth · 2021-07-21T19:13:18Z

I think either way would work.

I don't find the argument of consistency with VHDS that compelling. I think that functionality was added to VHDS because there was a use-case for it, but it's not clear to me that we have a corresponding use-case for LEDS. But if there is such a use-case, then I think that would be a strong argument.

I tend to favor clarity of the API over deprecation pain, but I do see that pain as well. Others may feel differently.

I'd be interested to hear what others think.

adisuissa · 2021-07-22T13:47:03Z

/cc @envoyproxy/api-shepherds
Currently in a Locality there's a field:

repeated LbEndpoint lb_endpoints = 2;

With the introduction of LEDS we can do one of the following:

Deprecate the field, and add a oneof around the LEDS config and a new message that has the repeated LbEndpoint (See comment above).
Pros: endpoints for a locality are configured either via LEDS or in the EDS response (single source of truth), which IMHO seems easier to control from an operations point-of-view.
Cons: churn due to field deprecation.
Keep lb_endpoints, and add the LEDS config field (See current change) so the locality's endpoints will essentially be merged from the 2 sources.
Pros: This is consistent with VHDS (and the virtual_hosts field), and for Envoy the code can easily support this.
Cons: If there isn't a use-case for this, then the API shouldn't allow it.

Any thoughts on this?

htuch · 2021-07-23T02:23:40Z

I think we could just require that they are mutually exclusive in documentation and enforce in code rather than proto structure.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2021-07-28T18:57:58Z

Deprecating in the proto structure is probably the right way to go (from API standpoint).
I've left a TODO to deprecate the lb_endpoints list and replace it as @markdroth suggested. This will be done once LEDS is operational.

markdroth · 2021-08-02T15:11:35Z

/lgtm api

adisuissa · 2021-08-02T17:50:20Z

@lizan @mattklein123 for non-googler review. PTAL

mattklein123

Thanks, LGTM.

…bridge-stream * upstream/main: (32 commits) tls: move ssl connection info into SocketAddressProvider (envoyproxy#17334) conn pool: default enable runtime feature `conn_pool_delete_when_idle` (envoyproxy#17577) api: LEDS api introduction (envoyproxy#17419) kafka: add support for api versions request in mesh-filter (envoyproxy#17475) ext_proc: Implement BUFFERED_PARTIAL processing mode (envoyproxy#17531) tooling: Async/pathlib/mypy cleanups and utils (envoyproxy#17505) xds: restructure CertificateProvider fields (envoyproxy#17201) Refactor OverloadIntegrationTest breaking out a test base, and the fake resource monitors. (envoyproxy#17530) listener: move active connection collection out of active tcp listener (envoyproxy#16947) tools: format checks for backticks (envoyproxy#17566) coverage: set lower limit for common/quic and common (envoyproxy#17573) v2: final source removal (envoyproxy#17565) test: bumping coverage (envoyproxy#17564) quic: enforcing header size and contents (envoyproxy#17520) Support for canonicalizing URI properly for AWS SigV4 signer (envoyproxy#17137) listener: add a stat for transport socket connect timeout (envoyproxy#17458) listener: add listen() error handling (envoyproxy#17427) http: return per route config when direct response is set (envoyproxy#17449) removing most v2 references from source/ (envoyproxy#17415) bug fix: return bootstrap when validating config (envoyproxy#17499) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

api: LEDS api introduction

174ad80

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot added the api label Jul 20, 2021

repokitteh-read-only bot assigned markdroth Jul 20, 2021

Adding missing proto files

56f44b0

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

markdroth reviewed Jul 21, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 21, 2021

Removing oneof

96a4ed8

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot removed the waiting label Jul 21, 2021

markdroth reviewed Jul 21, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 21, 2021

adisuissa added 4 commits July 27, 2021 10:55

Using oneof with a list

9d31e22

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into leds_api

5d49958

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Removing unused import

da0db40

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into leds_api

9e5bb43

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot removed the waiting label Jul 27, 2021

repokitteh-read-only bot removed the api label Aug 2, 2021

mattklein123 self-assigned this Aug 2, 2021

lizan approved these changes Aug 3, 2021

View reviewed changes

mattklein123 approved these changes Aug 4, 2021

View reviewed changes

mattklein123 merged commit 70887c6 into envoyproxy:main Aug 4, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

api: LEDS api introduction (envoyproxy#17419)

08ae38d

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Conversation

adisuissa commented Jul 20, 2021

Uh oh!

repokitteh-read-only bot commented Jul 20, 2021

Uh oh!

markdroth Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

adisuissa Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

markdroth Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

markdroth commented Jul 21, 2021

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markdroth commented Jul 21, 2021

Uh oh!

adisuissa commented Jul 22, 2021

Uh oh!

htuch commented Jul 23, 2021

Uh oh!

adisuissa commented Jul 28, 2021

Uh oh!

markdroth commented Aug 2, 2021

Uh oh!

adisuissa commented Aug 2, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adisuissa commented Jul 21, 2021 •

edited

Loading